Add GetCompatibilePackageTargetFrameworks Task to the new packageValidation package #7057

Anipik · 2021-03-05T23:48:40Z

GetCompatibilePackageTargetFrameworks tasks returns the list of tfms where a particular package should be installed and closure tested
we determine the compatible tfms by already defined compatible rules in the nuget and also prune the list by removing some older frameworks.
Added the tests as well.

src/Microsoft.DotNet.PackageValidation.Tests/Microsoft.DotNet.PackageValidation.Tests.csproj

src/Microsoft.DotNet.PackageValidation/Asset.cs

src/Microsoft.DotNet.PackageValidation/GetCompatibilePackageTargetFrameworks.cs

ViktorHofer · 2021-03-08T18:28:52Z

src/Microsoft.DotNet.PackageValidation/GetCompatibilePackageTargetFrameworks.cs

+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.NetCoreApp20);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.NetCoreApp21);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.NetCoreApp30);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.NetCoreApp31);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.Net50);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.Net45);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.Net451);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.Net452);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.Net46);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.Net461);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.Net462);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.Net463);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.NetStandard10);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.NetStandard11);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.NetStandard12);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.NetStandard13);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.NetStandard14);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.NetStandard15);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.NetStandard16);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.NetStandard17);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.NetStandard20);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.NetStandard21);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.UAP10);


Static known frameworks shouldn't be hardcoded in a compiled asset and instead be passed into the task and supplied via the targets file that invokes the task.

why would it be better to pass the static list, thats not gonna be changed, be provided as an input ?

In case we would ever want to mutate these items, we could do so via msbuild without the need to rebuild the assembly and flow it into dotnet/runtime or any other consumer repo.

Ie wouldn't we want to remove the UAP entry there when we stop supporting UAP entirely?

src/Microsoft.DotNet.PackageValidation/build/Microsoft.DotNet.PackageValidation.props

src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetComaptiblePackageTargetFrameworks.cs

src/Microsoft.DotNet.PackageValidation/GetCompatibilePackageTargetFrameworks.cs

src/Microsoft.DotNet.PackageValidation/build/Microsoft.DotNet.PackageValidation.props

src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetComaptiblePackageTargetFrameworks.cs

missymessa · 2021-03-08T19:35:01Z

We have an abstract class in Arcade to facilitate in DI for the MSBuild BuildTask classes: https://github.com/dotnet/arcade/blob/master/Documentation/Mechanics/MSBuildTaskDependencyInjection.md

src/Microsoft.DotNet.PackageValidation.Tests/Microsoft.DotNet.PackageValidation.Tests.csproj

ViktorHofer

I think this is a reasonable point to start. I think the list of hardcoded frameworks shouldn't be part of the compiled assembly but I might be missing context and there's actually a reason for not having it exposed in msbuild. Besides that, LGTM. Thanks Anirudh

Anipik · 2021-03-09T20:22:15Z

I think the list of hardcoded frameworks shouldn't be part of the compiled assembly but I might be missing context and there's actually a reason for not having it exposed in msbuild

The main reason is to have minimal inputs that a user has to specify while consuming in this package. I think we can make a final decision later when we know more about how this package is going to be consumed(part of sdk or standalone package) and how this fits as a consumer scenario.

Because dropping the tfm from the list might only be relevant to the runtime scenario where we run this to verify that there are no duplicated types, hence customer might not have to worry about consuming the latest package.

ViktorHofer · 2021-03-10T13:45:22Z

The main reason is to have minimal inputs that a user has to specify while consuming in this package. I think we can make a final decision later when we know more about how this package is going to be consumed(part of sdk or standalone package) and how this fits as a consumer scenario.

That shouldn't hinder you from defining the static list via msbuild items in the package's props file which is auto-imported when referencing the package and pass that list into the task when invoking it. You could either add a simple target that invokes the task or require consumers to pass that list into the task invocation.

ericstj

Sorry for the late feedback

ericstj · 2021-03-11T01:09:29Z

src/Microsoft.DotNet.PackageValidation/GetCompatiblePackageTargetFrameworks.cs

+            return result && !Log.HasLoggedErrors;
+        }
+
+        public static List<NuGetFramework> GetTestFrameworks(List<NuGetFramework> packageTargetFrameworks)


nit: this can be IEnumerable and you can avoid calls ToList

ericstj · 2021-03-11T01:11:33Z

src/Microsoft.DotNet.PackageValidation/GetCompatiblePackageTargetFrameworks.cs

+            }
+        }
+
+        public List<ITaskItem> CreateItemFromTestFramework(string title, string version, List<NuGetFramework> testFrameworks, string rids)


Similarly List vs IEnumerable. Try to avoid using concrete collections unless you actually need to. It allocates unnecessary arrays.

General rules:
If all you care about is enumeration use IEnumerable
If you need to enumerate many times, but don't need to modify results, consider an array.
If you need to modify the collection then use IList.

src/Microsoft.DotNet.PackageValidation/NupkgParser.cs

ericstj · 2021-03-11T01:19:33Z

src/Microsoft.DotNet.PackageValidation/NupkgParser.cs

+            PackageAsset asset = null;
+            if (filePath.StartsWith("ref"))
+            {
+                var stringParts = filePath.Split(@"/");


pull the split out of these if statements. Make sure you are doing length checks on the resultant array to avoid index out of range on packages with unexpected content (like a file named ref or a file under runtimes\foo.txt etc). When doing string comparisons use OrdinalIgnoreCase.

ericstj · 2021-03-11T01:28:11Z

src/Microsoft.DotNet.PackageValidation/GetCompatiblePackageTargetFrameworks.cs

+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.NetStandard17);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.NetStandard20);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.NetStandard21);
+            allTargetFrameworks.Add(FrameworkConstants.CommonFrameworks.UAP10);


Agree with what others have said about this, better to have this data in props. If it were easy to calculate it from NuGet that would be even better, but we found that wasn't the case so better to just define in props so it can more easily be controlled by caller.

ericstj · 2021-03-11T01:29:40Z

src/Microsoft.DotNet.PackageValidation/GetCompatiblePackageTargetFrameworks.cs

+        public string[] PackagePaths { get; set; }
+
+        [Output]
+        public ITaskItem[] TestProjects { get; set; }


Try to document your inputs and outputs. What's the expected identity and metadata and what is it used for. This is the tasks's public API.

Naming here is off, consider TestPackages

ericstj · 2021-03-11T01:30:41Z

src/Microsoft.DotNet.PackageValidation/GetCompatiblePackageTargetFrameworks.cs

+            List<ITaskItem> testprojects = new List<ITaskItem>();
+            foreach (var framework in testFrameworks)
+            {
+                var supportedPackage = new TaskItem(title);


title here is confusing. Shouldn't this be packageId?

ericstj · 2021-03-11T01:33:14Z

src/Microsoft.DotNet.PackageValidation/GetCompatiblePackageTargetFrameworks.cs

+                        rids.Add(item.Rid + "-x64");
+                }
+            }
+            return string.Join(";", rids);


Why are you joining here when other methods deal with lists/enumerables? Consider moving the join to the CreateItemFromTestFramework method where this is used.

ericstj · 2021-03-11T01:33:48Z

src/Microsoft.DotNet.PackageValidation/GetCompatiblePackageTargetFrameworks.cs

+
+        public string GetRidsFromPackage(Package package)
+        {
+            List<string> rids = new List<string>();


You can avoid a list allocation here by making this method return an enumerable and yield returning the items

* [master] Update dependencies from dotnet/xliff-tasks mono/linker (#7048) [master] Update dependencies from dotnet/xliff-tasks mono/linker * Post build sign algorithmic description (#7042) * Post build sign algorithmic description * Update Documentation/CorePackages/PostBuildSigning.md Co-authored-by: Christopher Costa <[email protected]> * Update Documentation/CorePackages/PostBuildSigning.md Co-authored-by: Christopher Costa <[email protected]> * Update Documentation/CorePackages/PostBuildSigning.md Co-authored-by: Christopher Costa <[email protected]> Co-authored-by: Christopher Costa <[email protected]> * Preview flow setup script (#6996) This script sets up flow for a .NET 6 preview given a few input parameters. * Revert "Saving sdk.txt using utf8" (#7051) This reverts commit 5624102. * Update dependencies from https://github.com/mono/linker build 20210304.2 (#7055) [master] Update dependencies from mono/linker * [master] Update dependencies from dotnet/roslyn dotnet/xharness (#7061) [master] Update dependencies from dotnet/roslyn dotnet/xharness * Add pipeline triggers for main branch (#7067) * remove triggers for master (#7069) * use main as the publishing branch (#7070) * Update dependencies from https://github.com/dotnet/arcade build 20210305.1 (#7072) [main] Update dependencies from dotnet/arcade * Add GetCompatibilePackageTargetFrameworks Task to the new packageValidation package (#7057) * add package valiation proj * adding tests * address feedback * delete extra file * correct typo * set XUnitPublishTargetFramework to net5.0 * use netcoreapp3.1 * Add 'checkDownloadedFiles: true' to all invocations of DownloadBuildArtifacts (#7066) * Use blob.core.windows instead of azureedge for Helix SDK / CLI acquisition (#7078) Since Helix agents tend to be in the same data center or geolocated in the Western US, this isn't a perf degradation (gain, in some cases) but also when executed at Helix scale, means a substantial savings * Add handling for new queues (#7079) * Send `System.PullRequest.TargetBranch` in job created by SendHelixJob task (#7075) * [API Compat] Add attribute diffing for generic and regular parameters (#7058) * Add attribute diffing for generic and regular parameters * PR Feedback (avoid duplication) * Mark internal channel configs as internal (#7090) * Update arcade for publishing fixes (#7093) * fix parameter name (#7094) * Update SDK's version to 6.0 Preview 2 (#7096) * [main] Update dependencies from dotnet/arcade mono/linker (#7081) [main] Update dependencies from dotnet/arcade mono/linker - Merge branch 'main' into darc-main-c7daeaa9-149a-434b-8e58-adbd726d8077 * Improve the M2M guide (#7091) * Use ubuntu & debian instead of unix & freebsd (#7089) * use ubuntu & debian instead of unix & freebsd * use linux instead of ubuntu * Avoid zipping VS templates (#7056) * Some meta data was dropping, so I included it so it wouldn't get dropped anymore (#7100) * Some meta data was dropping, so I included it so it wouldn't get dropped anymore * remove added whitespace * using tryGetValue to validate existence of data instead of containsKey * Documentation update for changes to Arcade Validation (#7063) * Documentation update for changes to Arcade Validation * Update documentation to use best judgement about validating against bellwether repos * Update Documentation/Validation/Overview.md Co-authored-by: Ricardo Arenas <[email protected]> * Update Documentation/Validation/Overview.md Co-authored-by: Ricardo Arenas <[email protected]> * Update Documentation/Validation/Overview.md Co-authored-by: Ricardo Arenas <[email protected]> * Tweaks per code review feedback Co-authored-by: Ricardo Arenas <[email protected]> * Add "call" to prevent the wrapper script from causing exit before AzDO reporter execution. (#7103) * Fix argument escaping when running Command (#7109) I found out the on Linux, commands were failing because every single one of them was escaped, so things like this would fail: ``` dotnet "tool" "install" "--version" ... ``` I need this change for #7029 * re-enable component governance (#7105) * [main] Update dependencies from dotnet/roslyn dotnet/xharness dotnet/arcade mono/linker (#7098) [main] Update dependencies from dotnet/roslyn dotnet/xharness dotnet/arcade mono/linker * Update badge branch names (#7112) * Add OneLocBuild template to arcade (#6977) * M2M Renaming Guide - Search for master not main (#7026) * Enable conditional facts/theories to reference static fields (#7117) * Add net472 to PackageValidation package (#7115) * add net472 to package * correcting the csproj * add net472 to tests * Add binaryLog option to SB Build command (#7104) * [main] Update dependencies from dotnet/arcade mono/linker (#7116) [main] Update dependencies from dotnet/arcade mono/linker * Add support for Mono AOT perf runs (#7123) * [main] Update dependencies from dotnet/arcade mono/linker (#7124) [main] Update dependencies from dotnet/arcade mono/linker * Add telemetry to generate-locproject.ps1 (#7122) * Update master -> main for perf scripts (#7127) * [main] Update dependencies from dotnet/xliff-tasks mono/linker (#7129) [main] Update dependencies from dotnet/xliff-tasks mono/linker * Fix the public key token in frameworkList (#7130) I happened to be referencing this code and noticed it dropped leading 0s from bytes when formatting the public key token. For example `b03f5f7f11d5a3a` `b03f5f7f11d50a3a` * [main] Update dependencies from dotnet/xliff-tasks mono/linker dotnet/xharness (#7131) * Update dependencies from https://github.com/dotnet/xliff-tasks build 20210319.2 XliffTasks From Version 1.0.0-beta.21168.1 -> To Version 1.0.0-beta.21169.2 * Update dependencies from https://github.com/mono/linker build 20210319.4 Microsoft.NET.ILLink.Tasks From Version 6.0.100-preview.2.21169.1 -> To Version 6.0.100-preview.2.21169.4 * Update dependencies from https://github.com/dotnet/xharness build 20210319.2 Microsoft.DotNet.XHarness.CLI From Version 1.0.0-prerelease.21162.1 -> To Version 1.0.0-prerelease.21169.2 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Add .NET 6 previews 3-5 (#7126) * Add .NET 6 previews 3-5 * Add VS 17.0 * Update OneLocBuild stuff for AzDO repos (#7136) Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Matt Mitchell <[email protected]> Co-authored-by: Christopher Costa <[email protected]> Co-authored-by: Ricardo Arenas <[email protected]> Co-authored-by: Anirudh Agnihotry <[email protected]> Co-authored-by: Matt Galbraith <[email protected]> Co-authored-by: Drew Scoggins <[email protected]> Co-authored-by: Łukasz Patalas <[email protected]> Co-authored-by: Santiago Fernandez Madero <[email protected]> Co-authored-by: Bill Wert <[email protected]> Co-authored-by: Viktor Hofer <[email protected]> Co-authored-by: Přemek Vysoký <[email protected]> Co-authored-by: Tomáš Matoušek <[email protected]> Co-authored-by: Missy Messa <[email protected]> Co-authored-by: Epsitha Ananth <[email protected]> Co-authored-by: Jon Fortescue <[email protected]> Co-authored-by: Genevieve Warren <[email protected]> Co-authored-by: Stephen Toub <[email protected]> Co-authored-by: Michael Simons <[email protected]> Co-authored-by: Eric StJohn <[email protected]> Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

…dation package (dotnet#7057) * add package valiation proj * adding tests * address feedback * delete extra file * correct typo * set XUnitPublishTargetFramework to net5.0 * use netcoreapp3.1

ViktorHofer · 2021-06-01T11:17:29Z

@Anipik are you planning to follow-up on the feedback provided by @ericstj and me (less allocation and no static list in code)?

Anipik · 2021-06-01T16:34:29Z

@Anipik are you planning to follow-up on the feedback provided by @ericstj and me (less allocation and no static list in code)?

yes i will try to get it later today.

Anipik requested review from ViktorHofer, ericstj, safern and joperezr March 5, 2021 23:48

ViktorHofer reviewed Mar 8, 2021

View reviewed changes

src/Microsoft.DotNet.PackageValidation.Tests/Microsoft.DotNet.PackageValidation.Tests.csproj Outdated Show resolved Hide resolved

ViktorHofer reviewed Mar 8, 2021

View reviewed changes

src/Microsoft.DotNet.PackageValidation/Asset.cs Outdated Show resolved Hide resolved

ViktorHofer reviewed Mar 8, 2021

View reviewed changes

src/Microsoft.DotNet.PackageValidation/Asset.cs Outdated Show resolved Hide resolved

ViktorHofer reviewed Mar 8, 2021

View reviewed changes

src/Microsoft.DotNet.PackageValidation/GetCompatibilePackageTargetFrameworks.cs Outdated Show resolved Hide resolved

ViktorHofer reviewed Mar 8, 2021

View reviewed changes

src/Microsoft.DotNet.PackageValidation/GetCompatibilePackageTargetFrameworks.cs Outdated Show resolved Hide resolved

ViktorHofer reviewed Mar 8, 2021

View reviewed changes

src/Microsoft.DotNet.PackageValidation/GetCompatibilePackageTargetFrameworks.cs Outdated Show resolved Hide resolved

ViktorHofer reviewed Mar 8, 2021

View reviewed changes

src/Microsoft.DotNet.PackageValidation/build/Microsoft.DotNet.PackageValidation.props Outdated Show resolved Hide resolved

Anipik mentioned this pull request Mar 8, 2021

use packages instead of package report for packageInstallation and closure verification dotnet/runtime#49315

Merged

Anipik added 3 commits March 8, 2021 10:53

add package valiation proj

562a178

adding tests

50516a9

address feedback

9628a4a

safern reviewed Mar 8, 2021

View reviewed changes

src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetComaptiblePackageTargetFrameworks.cs Outdated Show resolved Hide resolved

safern reviewed Mar 8, 2021

View reviewed changes

src/Microsoft.DotNet.PackageValidation/GetCompatibilePackageTargetFrameworks.cs Outdated Show resolved Hide resolved

safern reviewed Mar 8, 2021

View reviewed changes

src/Microsoft.DotNet.PackageValidation/build/Microsoft.DotNet.PackageValidation.props Show resolved Hide resolved

safern reviewed Mar 8, 2021

View reviewed changes

src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetComaptiblePackageTargetFrameworks.cs Outdated Show resolved Hide resolved

Anipik added 3 commits March 8, 2021 13:39

delete extra file

a698956

correct typo

aec63a5

set XUnitPublishTargetFramework to net5.0

2969a1d

ViktorHofer reviewed Mar 8, 2021

View reviewed changes

src/Microsoft.DotNet.PackageValidation.Tests/Microsoft.DotNet.PackageValidation.Tests.csproj Outdated Show resolved Hide resolved

ViktorHofer reviewed Mar 8, 2021

View reviewed changes

src/Microsoft.DotNet.PackageValidation.Tests/Microsoft.DotNet.PackageValidation.Tests.csproj Outdated Show resolved Hide resolved

Base automatically changed from master to main March 8, 2021 23:09

ViktorHofer approved these changes Mar 8, 2021

View reviewed changes

use netcoreapp3.1

6cc83ff

Anipik merged commit 2b3b963 into dotnet:main Mar 9, 2021

Anipik deleted the tfmValidation branch March 9, 2021 20:22

ericstj reviewed Mar 11, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GetCompatibilePackageTargetFrameworks Task to the new packageValidation package #7057

Add GetCompatibilePackageTargetFrameworks Task to the new packageValidation package #7057

Anipik commented Mar 5, 2021 •

edited

Loading

ViktorHofer Mar 8, 2021

Anipik Mar 8, 2021

ViktorHofer Mar 8, 2021

ViktorHofer Mar 8, 2021

missymessa commented Mar 8, 2021

ViktorHofer left a comment

Anipik commented Mar 9, 2021

ViktorHofer commented Mar 10, 2021 •

edited

Loading

ericstj left a comment

ericstj Mar 11, 2021

ericstj Mar 11, 2021

ericstj Mar 11, 2021

ericstj Mar 11, 2021

ericstj Mar 11, 2021

ericstj Mar 11, 2021

ericstj Mar 11, 2021

ericstj Mar 11, 2021

ericstj Mar 11, 2021

ViktorHofer commented Jun 1, 2021

Anipik commented Jun 1, 2021

Add GetCompatibilePackageTargetFrameworks Task to the new packageValidation package #7057

Add GetCompatibilePackageTargetFrameworks Task to the new packageValidation package #7057

Conversation

Anipik commented Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

missymessa commented Mar 8, 2021

ViktorHofer left a comment

Choose a reason for hiding this comment

Anipik commented Mar 9, 2021

ViktorHofer commented Mar 10, 2021 • edited Loading

ericstj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer commented Jun 1, 2021

Anipik commented Jun 1, 2021

Anipik commented Mar 5, 2021 •

edited

Loading

ViktorHofer commented Mar 10, 2021 •

edited

Loading